Skip to content

[ISSUE #19] Make HEALPix/MOC support optional #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Jul 11, 2023
Merged

[ISSUE #19] Make HEALPix/MOC support optional #26

merged 9 commits into from
Jul 11, 2023

Conversation

esabol
Copy link
Contributor

@esabol esabol commented Jul 6, 2023 •
edited
Loading

This PR address issue #19 and makes support for HEALPix/MOC functionality optional. It is enabled by default, but it can be disabled by including USE_HEALPIX=0 as an argument to the make commands. The README.pg_sphere file is updated to reflect this change (and other recent changes to the build process).

Also, when upgrading my installation from 1.2.1 to 1.2.2, I was getting the error:

# ALTER EXTENSION pg_sphere UPDATE TO '1.2.2';
ERROR:  function g_spoint3_fetch(internal, internal, internal) does not exist

I've included a fix for that in this PR (the function is just g_spoint3_fetch(internal)). Let me know if you prefer that I submit that as a separate PR.

@esabol esabol changed the title Make HEALPix/MOC support optional [ISSUE #19] Added PARALLEL SAFE mark to pgSphere functions Make HEALPix/MOC support optional Jul 6, 2023
@esabol esabol changed the title [ISSUE #19] Added PARALLEL SAFE mark to pgSphere functions Make HEALPix/MOC support optional [ISSUE #19] Make HEALPix/MOC support optional Jul 6, 2023
@esabol
Copy link
Contributor Author

esabol commented Jul 7, 2023

make test fails, but I don't think it worked before this PR either? Does it work for you? Does anyone care? make crushtest works fine...

@esabol esabol force-pushed the issue-19-make-healpix-moc-optional branch from ceab70d to 0d272eb Compare July 7, 2023 03:55
@vitcpp
Copy link
Contributor

vitcpp commented Jul 7, 2023 •
edited
Loading

@esabol Nice catch! You've found g_spoint3_fetch issue. Once we do not change the version number I'm ok to do it in this PR. Thank you!

@vitcpp
Copy link
Contributor

vitcpp commented Jul 7, 2023

@esabol Concerning make test. It fails without your PR. I will create an issue for it.

vitcpp
vitcpp reviewed Jul 7, 2023
View reviewed changes
Makefile Outdated
REGRESS += healpix moc mocautocast
endif

REGRESS += epochprop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose not to separate epochprop into a new line. It is already included unconditionally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't want to change the order of the tests, as I thought that might affect something. I think I saw a comment somewhere saying that "the order matters" and not to change it, but maybe that doesn't apply to REGRESS. If it's ok to change the order of the tests, it would certainly simplify things if I could add it unconditionally above this line and I will update the PR.

Copy link
Contributor

@vitcpp vitcpp Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may guess that epochprop independent on other tests. I tried to move epochprop back and forth and I haven't seen any problems with it. I got your point. It is up to you to keep PR unchanged. Furthermore, we are planning to cleanup and improve Makefile in the future. I do not see any problems with your PR at the moment.

@esabol esabol mentioned this pull request Jul 7, 2023
Closed
@esabol
Copy link
Contributor Author

esabol commented Jul 7, 2023 •
edited
Loading

With the help of Travis CI, I got make test working again, both with HEALPix and without. Some of the line numbers were just off by one. I've included these changes in this PR since the expected output for make test is different with USE_HEALPIX=0 and without, so I think it's related. This work addresses issue #28 as a result. I think we should keep init_test.sql. I think it's meant to be used to test the build before you install it. So perhaps the README should recommend that the procedure should be make; make test; make install; make installcheck; make crushtest?

I've also added make test and make crushtest to the Travis CI and added a separate USE_HEALPIX=0 build and it's testing. All this adds approximately 1 minute to each CI job or about 6 minutes for all builds in PR. I mention this since I know Travis CI costs money. These changes address issue #27.

That just leaves the minimum PostgreSQL version mentioned in the README.pg_sphere file. I've changed it to version 9.6 in the most recent commits. Other than that, this PR is ready to merge if you like what you see. Thanks!

@vitcpp
Copy link
Contributor

vitcpp commented Jul 8, 2023

@esabol You did great work! Give me please some time to review the changes. Thank you!

@vitcpp
Copy link
Contributor

vitcpp commented Jul 10, 2023

@esabol I tested your changes. It works for me with and without USE_HEALPIX. Thank you!

@esabol
Copy link
Contributor Author

esabol commented Jul 10, 2023

Thank you, @vitcpp !

@vitcpp vitcpp merged commit 2c4cd20 into postgrespro:master Jul 11, 2023
@esabol esabol mentioned this pull request Jul 11, 2023
Closed
@esabol esabol deleted the issue-19-make-healpix-moc-optional branch July 11, 2023 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitcpp vitcpp vitcpp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@esabol @vitcpp